Reviewable status: 1 change requests, 0 of 1 approvals obtained, 8 unresolved discussions (waiting on @DmytroMuravskyi)

TravelDistanceAnalyzer/dependencies/CirculationSegment.g.cs line 1 at r1 (raw file):

//----------------------

Please add *.g.cs files to the ignore list

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 32 at r1 (raw file):

        public AdaptiveGrid Build(IEnumerable<CirculationSegment> corridors,
                                  IEnumerable<SpaceBoundary> rooms,
                                  List<WallCandidate>? walls = null,

Why do you need to make List and List nullable? List is a reference type and can be null

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 32 at r1 (raw file):

        public AdaptiveGrid Build(IEnumerable<CirculationSegment> corridors,
                                  IEnumerable<SpaceBoundary> rooms,
                                  List<WallCandidate>? walls = null,

Can it be IEnumerable and IEnumerable?

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 67 at r1 (raw file):

            foreach (var room in _roomExits)
            {
                var p = room.Key.Boundary.Perimeter.TransformedPolygon(room.Key.Transform);

Try to alwaays use meaningful names (e.g. 'polygon' or 'roomBoundary' instead of 'p')

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 113 at r1 (raw file):

            }

            var mainConnection = _grid.GetVertex(exit.Edges.First().OtherVertexId(exit.Id));

Are you sure that exit.Edges always has at least one value? Maybe it makes sense to add an additional verification:

Code snippet:

if (exit.Edges.Count > 2 || exit.Edges.Count < 1)
{
    return;
}

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 148 at r1 (raw file):

                var directionIndex = -1;
                var dot = delta.Unitized().Dot(xDir);
                if (dot.ApproximatelyEquals(1, 0.01))

create a private tolerance const with 0.01 value

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 197 at r1 (raw file):

        }

        private Edge? ClosestEdgeOnElevation(Vector3 location, out Vector3 point)

It is better to add the action name into the function name.

Edge? FindClosestEdgeOnElevation(Vector3 location, out Vector3 point)

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 243 at r1 (raw file):

        /// <param name="centerlines">Corridor segments with precalculated center lines.</param>
        /// <param name="grid">AdaptiveGrid to insert new vertices and edge into.</param>
        private void Intersect(List<(CirculationSegment Segment, Polyline Centerline)> centerlines)

CreateConnectionEdges or CreateIntersections

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 8 unresolved discussions (waiting on @katehryhorenko)

TravelDistanceAnalyzer/dependencies/CirculationSegment.g.cs line 1 at r1 (raw file):

HyparSpace stores *.g.cs in all functions. There is actually a benefit to that as every commit can be built even if schema is changed.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 32 at r1 (raw file):

It's to prevent the warnings, I wanted the function to have 0 warnings and when null is assigned to non nullable type - compiler always produces one. There is also a logical difference: if walls null - Walls function is not in workflow. If empty list - function is there but there are no walls. The same is true for doors.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 32 at r1 (raw file):

Done.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 67 at r1 (raw file):

Done.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 113 at r1 (raw file):

Grid should not have free vertices, this is exceptional situation. If this is possible, I want the exception, and not silent return.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 148 at r1 (raw file):

Done.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 197 at r1 (raw file):

Done.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 243 at r1 (raw file):

Done.

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 8 unresolved discussions (waiting on @DmytroMuravskyi)

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 32 at r1 (raw file):

I see. So id you are planning to call Build method only with two input parameters (Build(corridors, rooms)), then yes, it's a correct approach. But I can see why you are getting the warning. Lets talk tomorrow :)

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 113 at r1 (raw file):

Then it's better to check the edges count and throw the exception at the begin of the function. You don't need to wait for exit.Edges.First() throw an exception

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 8 unresolved discussions (waiting on @katehryhorenko)

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 32 at r1 (raw file):

Yes, let's discuss. No, it's not about default input parameters, I removed them, as they are no longer used. It's about optional model dependencies: when doors or walls are not there they are null and this is much cleaner than empty list.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 113 at r1 (raw file):

Done.

Hey! Overall looks good!
I added a few comments - suggestions. You can ignore them.

I few comments that I would ask you to fix:

• Keep the order of the class members. My suggestion is
  {
  private cons
  private fields
  constructor
  public properties
  public methods
  private methods
  }
  But do not mix public methods and private methods :)
• Some methods have comments for parameters that these methods do not have.

Comments on anything public are welcome!

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 11 unresolved discussions (waiting on @DmytroMuravskyi)

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 104 at r4 (raw file):

                }
            }
            return exitVertex == null ? 0u : exitVertex.Id;

It's an alternative: exitVertex?.Id ?? 0u
But it's up to you what to use :)

Code quote:

exitVertex == null ? 0u : exitVertex.Id

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 248 at r4 (raw file):

        /// </summary>
        /// <param name="centerlines">Corridor segments with precalculated center lines.</param>
        /// <param name="grid">AdaptiveGrid to insert new vertices and edge into.</param>

Method doesn't have 'grid' input parameter

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 302 at r4 (raw file):

                        }

                        if (closestLeftProximity == -1 || closestRightProximity == -1)

Just a suggestion

Code snippet:

private const int InvalidProximity = -1;
private const int MaxProximityDifference = 2;

// ...

if (closestLeftProximity == InvalidProximity || closestRightProximity == InvalidProximity)
{
    continue;
}

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 375 at r4 (raw file):

        }

        private Edge? FindOnCollinearEdges(GridVertex start, ulong endId, Vector3 direction, Vector3 destination)

FindCollinearEdgeTowardsEndpoin

Code quote:

 FindOnCollinearEdges

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 379 at r4 (raw file):

            while (start.Id != endId)
            {
                GridVertex otherVertex = start;

The variable name otherVertex might be misleading because it's actually the next vertex in the loop. Consider renaming it to something like nextVertex for clarity.

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 428 at r4 (raw file):

        }

        private void ExtendToCorridor(Line l, CirculationSegment segment)

line

Code quote:

l

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 436 at r4 (raw file):

                var trimLine = new Line(l.Start - l.Direction() * maxDistance,
                                        l.End + l.Direction() * maxDistance);
                var inside = trimLine.Trim(transformedPolygon, out _);

intersectionLines

Code quote:

inside

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 454 at r4 (raw file):

        /// </summary>
        /// <param name="room">Room geometry.</param>
        /// <param name="centerlines">Corridor segments with precalculated center lines.</param>

Please fix the comment. 'grid' and 'centerLines' parameters do not exist :)

TravelDistanceAnalyzer/src/AdaptiveGridBuilder.cs line 462 at r4 (raw file):

            IEnumerable<Door>? doors)
        {
            var roomExits = new List<GridVertex>();

roomExitVertices

Code quote:

roomExits

Reviewable status: 1 of 1 approvals obtained, 10 unresolved discussions (waiting on @DmytroMuravskyi)